feat: Add mathematical operators for IntervalYearMonth type#11612
feat: Add mathematical operators for IntervalYearMonth type#11612pramodsatya wants to merge 1 commit intofacebookincubator:mainfrom
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @pramodsatya.
We can use more templates in this code I feel to reduce the volume of new code. PTAL.
pramodsatya
force-pushed
the
ym_multiply
branch
from
d1bac96 to
34c093e
Compare
|
Thanks @aditi-pandit, updated the PR to use more templates and reduce the lines of new code. Could you please take another look? |
pramodsatya
force-pushed
the
ym_multiply
branch
from
34c093e to
0afc30c
Compare
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
@pramodsatya : The code changes look good.
Is this code tested exhaustively by the expression fuzzer ? Also with the PrestoQueryRunner ? Would be great if you could paste the results.
| if constexpr (std::is_same_v<TResult, int64_t>) { | ||
| min = kLongMin; | ||
| max = kLongMax; | ||
| maxCheck = kMaxDoubleBelowInt64Max; |
There was a problem hiding this comment.
Rename to maxResult.
pramodsatya
force-pushed
the
ym_multiply
branch
from
0afc30c to
b1bf064
Compare
Thanks @aditi-pandit, the expression fuzzer CI failure is because the newly added function signature for |
|
@pramodsatya : Can you ping Tim about the Presto PR ? |
|
@aditi-pandit @pramodsatya can anyone else review and approve this PR since Tim is out? Thanks. |
pramodsatya
force-pushed
the
ym_multiply
branch
from
b1bf064 to
8194064
Compare
|
@aditi-pandit, the expression fuzzer CI is passing now, could you please take another look? |
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @pramodsatya
|
@Yuhta : Please can you help review. |
|
@Yuhta : can you help to review?Thanks. |
majetideepak
added
the
ready-to-merge
pramodsatya
force-pushed
the
ym_multiply
branch
from
8194064 to
6158737
Compare
|
@kKPulla has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…incubator#11612) Summary: Add support for mathematical functions `plus`, `minus`, `multiply`, and `divide` with `IntervalYearMonth` type. The function signatures added match that of [Presto](https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/type/IntervalYearMonthOperators.java), accounting for the Presto function signature changes in prestodb/presto#24089. Pull Request resolved: facebookincubator#11612 Reviewed By: pedroerp Differential Revision: D72997442 Pulled By: kKPulla fbshipit-source-id: 5fcf40639ed0af63e6ec198994aeaef49ba367cb
5 participants
Add support for mathematical functions
plus,minus,multiply, anddividewith
IntervalYearMonthtype. The function signatures added match that of Presto,accounting for the Presto function signature changes in prestodb/presto#24089.